Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Standard Library Modules: Fix spurious _Init_locks dllexport #3233

Merged
merged 2 commits into from
Nov 18, 2022
Merged

Standard Library Modules: Fix spurious _Init_locks dllexport #3233

merged 2 commits into from
Nov 18, 2022

Conversation

StephanTLavavej
Copy link
Member

I observed a curious message "Creating library meow.lib and object meow.exp" when consuming import std; with VS 2022 17.5 Preview 1. This doesn't repro when using the 17.5p1 compiler to consume import std; from microsoft/STL main; it repros only with the shipped machinery. I haven't figured out what the difference is yet, but I did find the root cause of the message.

The problem is that when we inject stacktrace.cpp into the import lib, it drags in yvals.h and the headache-inducing _Init_locks. In #3019, we guarded _Init_locks with _CRTBLD so it wouldn't affect users of stl/inc, but the problem here is that stacktrace.cpp is part of the STL's build, so _Init_locks is being emitted.

I have two fixes for this (either alone would be sufficient but I want to be extra sure here). First, stacktrace.cpp directly includes yvals.h but barely needs it. We can replace its use of _STL_VERIFY with direct calls to std::abort(), since these should only fail due to STL-internal logic errors (or something catastrophic happening at runtime).

Second, let's take the additional effort to fully remove _Init_locks from stl/inc and move it into an stl/src source-header that is then included by only the files that need _Init_locks - never those injected into the import lib.

(I experimented with a third strategy, enforcing all import lib files to be core-only via the recently-added _ENFORCE_ONLY_CORE_HEADERS. This is currently not possible, even after fixing stacktrace.cpp, due to nothrow.cpp including <new> (which would be fairly easy to disconnect) and locale0_implib.cpp eventually including <xfacet> which would be harder and scarier to rework. For the time being, I have abandoned that approach.)

Note: Unlike user-visible headers, our source-headers can be very lightweight, i.e. they don't need push-pop defenses and all that. We're currently inconsistent about how to give them idempotency guards, but I went with the lightweight #pragma once.

Here is the full repro that shows how I tracked this down to stacktrace.cpp:

C:\Temp\REPRO>"C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Auxiliary\Build\vcvars64.bat"
**********************************************************************
** Visual Studio 2022 Developer Command Prompt v17.5.0-pre.1.0
** Copyright (c) 2022 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x64'

C:\Temp\REPRO>type meow.cpp
import std;
int main() {
    std::cout << "Hello, modules world!\n";
}

C:\Temp\REPRO>cl /EHsc /nologo /W4 /std:c++latest /MDd /Od /c "C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32019\modules\std.ixx"
std.ixx

C:\Temp\REPRO>cl /EHsc /nologo /W4 /std:c++latest /MDd /Od meow.cpp std.obj
meow.cpp
   Creating library meow.lib and object meow.exp

C:\Temp\REPRO>meow.exe
Hello, modules world!

C:\Temp\REPRO>dumpbin /exports meow.exe | findstr "_Init_locks"
          1    0 000019A0 ??4_Init_locks@std@@QEAAAEAV01@AEBV01@@Z

C:\Temp\REPRO>copy "C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.35.32019\lib\x64\msvcprtd.lib" .
        1 file(s) copied.

C:\Temp\REPRO>lib /list msvcprtd.lib | findstr ".obj"
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\xonce2.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\xcharconv_tables_float.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\xcharconv_tables_double.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\xcharconv_ryu_tables.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\vector_algorithms.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\syserror_import_lib.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\stacktrace.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\sharedmutex.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\nothrow.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\locale0_implib.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\format.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\filesystem.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\charconv.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\asan_noop.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\alias_init_once_complete.obj
D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\alias_init_once_begin_initialize.obj

C:\Temp\REPRO>for %I in (alias_init_once_begin_initialize.obj alias_init_once_complete.obj asan_noop.obj charconv.obj filesystem.obj format.obj locale0_implib.obj nothrow.obj sharedmutex.obj stacktrace.obj syserror_import_lib.obj vector_algorithms.obj xcharconv_ryu_tables.obj xcharconv_tables_double.obj xcharconv_tables_float.obj xonce2.obj) do @lib /nologo msvcprtd.lib /extract:D:\a\_work\1\s\Intermediate\crt\github\stl\msbuild\stl_base\xmd\msvcp_kernel32\msvcp.nativeproj\objd\amd64\%I /out:extracted_%I

C:\Temp\REPRO>for %I in (extracted_*.obj) do @(dumpbin /symbols %I | findstr "_Init_locks" && echo Found in %I)
3E9 00000000 SECT131 notype ()    External    | ??4_Init_locks@std@@QEAAAEAV01@AEBV01@@Z (public: class std::_Init_locks & __cdecl std::_Init_locks::operator=(class std::_Init_locks const &))
Found in extracted_stacktrace.obj

⚠️ Note to self:

This will require MSVC-internal changes to add the new source-header.

@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Nov 16, 2022
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner November 16, 2022 06:10
@CaseyCarter
Copy link
Contributor

I experimented with a third strategy...

Thanks for mentioning this and saving me the trouble of asking =)

Here is the full repro that shows how I tracked this down...

Should we have a validation step in our build that looks for _Init_locks::operator=(const _Init_locks&) in the object files that we stuff into the import library?

@StephanTLavavej
Copy link
Member Author

Thanks for mentioning this and saving me the trouble of asking =)

😸

Should we have a validation step in our build that looks for _Init_locks::operator=(const _Init_locks&) in the object files that we stuff into the import library?

I wouldn't object to that, but (at least for me) I think it'd take more time to learn how to implement than it would be worth. With this PR, init_locks.hpp is a leaf, so it would be pretty hard to unintentionally drag into the import lib. If we mess it up again, I hereby grant you permission to say "I told you so" 😹

(The core-header-only approach may be feasible in the long term, which I'd want to explore if we keep adding import lib objects at a steady rate.)

@CaseyCarter
Copy link
Contributor

Should we have a validation step in our build that looks for _Init_locks::operator=(const _Init_locks&) in the object files that we stuff into the import library?

I wouldn't object to that, but (at least for me) I think it'd take more time to learn how to implement than it would be worth. With this PR, init_locks.hpp is a leaf, so it would be pretty hard to unintentionally drag into the import lib. If we mess it up again, I hereby grant you permission to say "I told you so" 😹

All true. I really don't want a repeat of the fiasco getting the LKG update out, but it would be quite hard to do so unintentionally now. (This statement of agreement should not be construed as waiving my right to say "I told you so".)

@StephanTLavavej StephanTLavavej self-assigned this Nov 18, 2022
@StephanTLavavej
Copy link
Member Author

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit 47c90ae into microsoft:main Nov 18, 2022
@StephanTLavavej StephanTLavavej deleted the stacktrace-init-locks branch November 18, 2022 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working modules C++23 modules, C++20 header units
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@StephanTLavavej @CaseyCarter